child_process: emit IPC messages on next tick #6909

cjihrig · 2016-05-20T23:15:37Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal.

Refs: #6561
Refs: #6902

R= @santigimeno @bnoordhuis

santigimeno · 2016-05-20T23:24:38Z

I like this solution better.

LGTM if CI agrees.

bnoordhuis · 2016-05-21T09:00:28Z

LGTM. This is also a fix for #3072, I think? Is it possible to write a reliable regression test?

santigimeno · 2016-05-21T14:06:20Z

I have a test: santigimeno@c07eae8, that passes with this change, and never exits without it. Feel free to use it / modify it if it works for you.

cjihrig · 2016-05-21T14:08:47Z

Test LGTM. I'll pull it into this PR.

jasnell · 2016-05-21T14:39:22Z

LGTM with the test added and green CI

bnoordhuis · 2016-05-21T16:38:06Z

Test LGTM2. @cjihrig Consider stress-testing it before you land the PR.

cjihrig · 2016-05-23T13:42:16Z

Normal CI: https://ci.nodejs.org/job/node-test-pull-request/2742/. Will stress test next.

cjihrig · 2016-05-23T15:21:46Z

The test timed out on Windows due to the different scheduling policy. I've added another commit, which addresses it, but for whatever reason, GitHub isn't currently showing it.

Here it is. @bnoordhuis and @santigimeno what do you think?

--- a/lib/cluster.js
+++ b/lib/cluster.js
@@ -719,7 +719,11 @@ function workerInit() {
       const handle = handles[key];
       delete handles[key];
       waitingCount++;
-      handle.owner.close(checkWaitingCount);
+
+      if (handle.owner)
+        handle.owner.close(checkWaitingCount);
+      else
+        handle.close(checkWaitingCount);
     }

     checkWaitingCount();

cjihrig · 2016-05-23T16:54:50Z

GitHub seems to be working again. CI after latest commit: https://ci.nodejs.org/job/node-test-pull-request/2744/

cjihrig · 2016-05-23T17:56:57Z

CI is green, except for one PPC machine that seems hung or something.

santigimeno · 2016-05-23T20:33:42Z

LGTM.

The last commit seems to fix a different issue though. A couple of comments:

Is SCHED_RR usable on Windows? or is it simply not recommended? If the latter, maybe the SchedulingPolicy of the cluster could be set unconditionally to RR in the test so it works the same on every platform.
Maybe it's worth adding a different test that exercises the ownerless handles problem on all the platforms, not only on Windows. I have created this if you want to use it: santigimeno@e8f284b

cjihrig · 2016-05-24T14:33:41Z

CI with all four commits: https://ci.nodejs.org/job/node-test-pull-request/2765/

cjihrig · 2016-05-24T15:37:35Z

CI was all green except for one flake (test-stdout-close-catch - #6918).

Stress test (Ubuntu): https://ci.nodejs.org/job/node-stress-single-test/737/
Stress test (Windows): https://ci.nodejs.org/job/node-stress-single-test/738/

bnoordhuis · 2016-05-24T15:57:03Z

LGTM. Looks like a flake on the fbsd10 buildbot.

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

The test in this commit runs correctly if IPC messages are properly consumed and emitted. Otherwise, the test times out. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a worker is disconnecting, it shuts down all of the handles it is waiting on. It is possible that a handle does not have an owner, which causes a crash. This commit closes such handles without accessing the missing owner. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

This test checks that ownerless cluster worker handles are closed correctly on disconnection. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

cjihrig · 2016-05-24T16:24:55Z

Thanks all. Landed in d59917b, dd21bd9, aadfe6c, and f0a07d9.

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

The test in this commit runs correctly if IPC messages are properly consumed and emitted. Otherwise, the test times out. Fixes: nodejs#6561 PR-URL: nodejs#6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-06-03T01:40:43Z

@cjihrig lts?

cjihrig · 2016-06-03T01:56:00Z

IMO yes. It keeps the IPC channel from getting constipated.

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

The test in this commit runs correctly if IPC messages are properly consumed and emitted. Otherwise, the test times out. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a worker is disconnecting, it shuts down all of the handles it is waiting on. It is possible that a handle does not have an owner, which causes a crash. This commit closes such handles without accessing the missing owner. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

This test checks that ownerless cluster worker handles are closed correctly on disconnection. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently, if an IPC event handler throws an error, it can cause the message to not be consumed, leading to messages piling up. This commit causes IPC events to be emitted on the next tick, allowing the channel's processing logic to move forward as normal. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

The test in this commit runs correctly if IPC messages are properly consumed and emitted. Otherwise, the test times out. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When a worker is disconnecting, it shuts down all of the handles it is waiting on. It is possible that a handle does not have an owner, which causes a crash. This commit closes such handles without accessing the missing owner. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

This test checks that ownerless cluster worker handles are closed correctly on disconnection. Fixes: #6561 PR-URL: #6909 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: nodejs#6909 Refs: nodejs#13459 Refs: nodejs#13648 PR-URL: nodejs#13856 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

This commit fixes a regression related to IPC 'message' events. When messages are not emitted in the next tick, a 'message' handler that throws can break the IPC read loop. Refs: #6909 Refs: #13459 Refs: #13648 PR-URL: #13856 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 20, 2016

cjihrig mentioned this pull request May 20, 2016

cluster: guard against undefined message handlers #6902

Merged

2 tasks

cjihrig force-pushed the next-tick branch from 2530c01 to 61501bc Compare May 23, 2016 13:40

cjihrig force-pushed the next-tick branch 2 times, most recently from c5efa43 to 7cfabab Compare May 23, 2016 16:34

cjihrig force-pushed the next-tick branch from 7cfabab to 03a5cac Compare May 24, 2016 14:32

cjihrig and others added 4 commits May 24, 2016 12:20

cjihrig force-pushed the next-tick branch from 03a5cac to f0a07d9 Compare May 24, 2016 16:22

cjihrig merged commit f0a07d9 into nodejs:master May 24, 2016

cjihrig deleted the next-tick branch May 24, 2016 16:24

MylesBorins mentioned this pull request Jun 2, 2016

Audit commits not found on v4.x-staging #7116

Closed

MylesBorins added the lts-watch-v4.x label Jun 3, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

cjihrig mentioned this pull request Jun 21, 2017

child_process: fix IPC 'message' event regression #13856

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: emit IPC messages on next tick #6909

child_process: emit IPC messages on next tick #6909

cjihrig commented May 20, 2016

santigimeno commented May 20, 2016

bnoordhuis commented May 21, 2016

santigimeno commented May 21, 2016 •

edited

Loading

cjihrig commented May 21, 2016

jasnell commented May 21, 2016

bnoordhuis commented May 21, 2016

cjihrig commented May 23, 2016

cjihrig commented May 23, 2016 •

edited

Loading

cjihrig commented May 23, 2016

cjihrig commented May 23, 2016

santigimeno commented May 23, 2016 •

edited

Loading

cjihrig commented May 24, 2016

cjihrig commented May 24, 2016

bnoordhuis commented May 24, 2016

cjihrig commented May 24, 2016

MylesBorins commented Jun 3, 2016

cjihrig commented Jun 3, 2016

child_process: emit IPC messages on next tick #6909

child_process: emit IPC messages on next tick #6909

Conversation

cjihrig commented May 20, 2016

Checklist

Affected core subsystem(s)

Description of change

santigimeno commented May 20, 2016

bnoordhuis commented May 21, 2016

santigimeno commented May 21, 2016 • edited Loading

cjihrig commented May 21, 2016

jasnell commented May 21, 2016

bnoordhuis commented May 21, 2016

cjihrig commented May 23, 2016

cjihrig commented May 23, 2016 • edited Loading

cjihrig commented May 23, 2016

cjihrig commented May 23, 2016

santigimeno commented May 23, 2016 • edited Loading

cjihrig commented May 24, 2016

cjihrig commented May 24, 2016

bnoordhuis commented May 24, 2016

cjihrig commented May 24, 2016

MylesBorins commented Jun 3, 2016

cjihrig commented Jun 3, 2016

santigimeno commented May 21, 2016 •

edited

Loading

cjihrig commented May 23, 2016 •

edited

Loading

santigimeno commented May 23, 2016 •

edited

Loading